- 
                Notifications
    
You must be signed in to change notification settings  - Fork 182
 
          Add a parser for module-info-patch.maven files where `--add-exports and similar options can be specified more easily
          #963
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 
           Maybe @slawekjaranowski as a reviewer here too? Like #959 the decision about accepting a  Note: an email has been sent to the developer mailing list about those issues, but got no reaction yet.  | 
    
| 
           I'd like to experiment switching JLine's master branch to the new plugins, since i'm migrating it to Maven 4 with JPMS support.  | 
    
| 
           Documentation about this feature has been added in #976, in particular in the   | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for parsing module-info-patch.maven files that provide a more convenient way to specify module system options like --add-exports and --add-reads for testing purposes. Instead of overwriting module-info.java files in test directories or configuring verbose XML in the POM, developers can use a Maven-specific syntax that maps directly to Java compiler options.
Key changes:
- Adds a new parser for 
module-info-patch.mavenfiles with Java-like syntax for module system options - Integrates the parser into the test compilation workflow to generate appropriate compiler flags
 - Updates existing code to handle the new patch-based approach while maintaining backward compatibility
 
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description | 
|---|---|
src/test/resources/org/apache/maven/plugin/compiler/module-info-patch.maven | 
Example patch file showing the supported syntax and options | 
src/test/java/org/apache/maven/plugin/compiler/ModuleInfoPatchTest.java | 
Test class validating the parser functionality and output generation | 
src/main/java/org/apache/maven/plugin/compiler/ToolExecutorForTest.java | 
Integration of patch file parsing into the test compilation process | 
src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java | 
Updated comment clarifying main code handling | 
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java | 
Updated deprecation warnings to reference the new patch file approach | 
src/main/java/org/apache/maven/plugin/compiler/ModuleInfoPatchException.java | 
Exception class for patch file parsing errors | 
src/main/java/org/apache/maven/plugin/compiler/ModuleInfoPatch.java | 
Main parser implementation for module-info-patch.maven files | 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
        
          
                src/main/java/org/apache/maven/plugin/compiler/ModuleInfoPatch.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      …` and similar options can be specified more easily.
Co-authored-by: Copilot <[email protected]>
7d9f47e    to
    653b2fd      
    Compare
  
    …maven.compiler.preview=true` option is passed to the `mvn` command. This commit also adds an integration test.
| 
           I added a commit for enabling the parsing of  As for the similar commit on #959, this is an attempt to make this pull request acceptable.  | 
    
| 
           Removed the  I have the feeling that we will not have feedback before peoples start using this feature. Are we ready to merge (on a community point of view)?  | 
    
| 
           @desruisseaux Please assign appropriate label to PR according to the type of change.  | 
    
This is a variation of the practice consisting in overwriting the
module-info.javaof the main code with anothermodule-info.javadefined in the test directory. Instead, themodule-info.javafile formerly defined in the test would be replaced by amodule-info-patch.mavenfile in the same directory. The latter is Maven-specific: the content ofmodule-info-patch.mavenlooks likemodule-info.java, but is not standard Java, hence the.mavenfile suffix. The principles are:module-info.javafile for testing purposes is declared inmodule-info-patch.maven.module-info.javais not inmodule-info-patch.mavenneither. In particular, everything that specify paths to JAR files stay in thepom.xmlfile.patch-moduleblock of that file map directly to Java compiler or Java launcher options.The keywords are
add-modules,limit-modules,add-reads,add-exportsandadd-opens. All these keywords map tojavacoptions of the same names. Note that they are options where the values are package or module names, not paths to source or binary files. Options with path values should be handled in the<sources>and<dependencies>elements of the POM instead. Below is an example of amodule-info-patch.mavenfile content for modifying themodule-infoof a module namedmy.product.foo:Why this approach
It is not convenient to put these options in the plugin configuration in the POM because these options need to be specified on a module-per-module (in JPMS sense) basis, while the plugin configuration in the POM applies to the whole module source hierarchy. Actually, the options of all modules end up merged together in a single set of options for
javac, but in a more verbose form for differentiating each module. For example, if the optionadd-reads org.junit.jupiter.apiis declared inside themy.product.foomodule, it becomes--add-reads my.product.foo=org.junit.jupiter.apiin the options given tojavac. It is easer for developers to read and maintain their code if this verbosity is managed by the compiler plugin, in addition of avoiding the XML verbosity of the POM.Another demonstration that options declared in plugin configuration are impracticable is that many developers prefer to provide a
module-info.javain their test which override themodule-info.javaof their main code. But this approach has problems: the testmodule-info.javamust duplicate everything that is in the mainmodule-info.java. If they diverge, the tests may not behave as the main code would do. Furthermore, this approach sometime requires hacks in the compiler plugin such as pretending that the tests are the main code and the main code is a patch applied over the tests. For avoiding those issues,javacoptions such as--add-readsshould be used instead, but they are tedious to write and maintain as plugin configuration in the POM.Impact on other plugins
Other plugins do not need to parse this
module-info-patch.mavenfile. Only the Maven Compiler Plugin needs to do so. The compiler plugin generates aMETA-INF/maven/module-info-patch.argsfile which would be consumed by the Surefire plugin for executing the tests. Other plugins would need to care about this file only if they have something to do with tests. Note that Maven 3 already generates a file for JPMS arguments, but with a different content. So this approach is not really new.Gradual approach proposal
We could take a gradual approach by accepting this pull request with an amendment: if a
module-info-patch.mavenfile is found, print a warning saying that this is an experimental feature. I think that the risk is low because most users will probably not experiment this feature until the Surefire is upgraded for consuming the above-citedMETA-INF/maven/module-info-patch.argsfile. But having feedbacks from peoples willing to try on the command-line would be very valuable.